Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for relative requirements in pip_install #433

Merged
merged 10 commits into from
Oct 26, 2021

Conversation

aaliddell
Copy link
Contributor

Alternate to #367 with a better implementation, thanks to @LouisStAmour

PR Checklist

Please check if your PR fulfills the following requirements:

  • Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: closes #366

Relative requirements do not work due to pip wheel being run from the generated repository root.

What is the new behavior?

Pip is now run with the working directory being the folder containing the requirements.txt file, with --wheel-dir pointed back at the correct location to write the wheels.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

A new example has been added to satisfy the desire for this to be tested from the other PR. I'm happy to shuffle this around elsewhere if you prefer.

This allows for relative requirements to be resolved in the way
that standalone pip would without patching or parsing the
requirements file manually
Prior to this, errors would be written to logs in the temp
directory that would promptly be deleted on parent test
teardown. This means at least the test logs will be printed
on failure
@amirh
Copy link

amirh commented Mar 12, 2021

Note that this fixes #434 as well

@amirh
Copy link

amirh commented Mar 13, 2021

Also note that this introduces another issue - I see that it ends up creatomg a build folder with the wheel directly within the source repository. Other than being a little dirty, if you have a BUILD and the file system is cases insensitive file in that same directory the wheel creation fails (can't create builddirectory) and this eventually fails pip_install. My workaround with this patch was to rename my bazel BUILD file to BUILD.bazel but obviously that's not a solution.

@LouisStAmour
Copy link

@amirh Could you help me reproduce this? Is it a specific version of pip by any chance?

https://github.com/pypa/pip/blob/9624d0d529243354bda9820f0cc12257cbdf47f0/src/pip/_internal/utils/temp_dir.py#L21 seems to indicate that current versions of pip will create a system-temporary directory named "pip-req-build-something", such as (on macOS): /private/tmp/pip-req-build-khv3ctud or similar and I tried multiple versions of the code using fs_events on macOS to monitor, and each time it seemed to create such a folder with no generic build directory?

On the off chance that it helps, here's a branch/variation where it creates a temp directory and tries to assign it to both the deprecated --build command line and blindly overwrites the TMPDIR environment variable. I wouldn't perhaps merge this as-is, but maybe it would be useful for testing purposes? LouisStAmour@df6ea4d

@LouisStAmour
Copy link

LouisStAmour commented Mar 13, 2021

pypa/pip@4bfc92d#diff-e2254827ebcf391f75d2d5e58c4e68e7f635bebd01eda913445f9ef339efed07 it does look like the earliest version of the wheel command uses a build_prefix folder defined over in https://github.com/pypa/pip/blob/4bfc92ddba8002f97a2b06e859537258a38792ff/pip/locations.py#L30 so my version of the code which assigns a temp folder to --build might be useful here, and ignored in later versions of pip. Note that the early version of the code puts the build folder in sys.prefix which doesn't make sense to me, but maybe in later versions this was changed to cwd... I am not a pip expert, I just play one in GH issues ;-)

@aaliddell
Copy link
Contributor Author

I can't seem to recreate the behaviour of it producing a build directory in the source tree. It appears it might be a combination of running with a venv activated with an older pip version, if I'm understanding correctly?

@amirh would you be able to produce a minimal reproduction with details on the OS version, Python version and pip version?

@jvolkman
Copy link
Contributor

+1 for something like this change. I've got a wheelhouse committed to my repo and couldn't find any straightforward way to reference it in a --find-links argument, either in requirements.txt or extra_pip_args. For now I've written a repository rule that takes requirements.txt and a list of workspace-relative paths as input, and generates a file with absolute --find-links lines written to the end. Not horrible, but this would be much better.

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good and self-contained to me, thanks!

@alexeagle
Copy link
Collaborator

FYI @aaliddell @jvolkman https://github.com/bazelbuild/rules_python/pull/807/files#diff-1ec451da56ea684a54fecd674f47424c763b5809868340bcfbffa7833f4884b0 proposes to remove this feature, so you might want to comment there if you've got a good reason to reconsider that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support local wheel files in pip_install requirements.txt
6 participants